fix(testing): revert change & fix playwright tests #4310

jsjoeio · 2021-10-04T23:19:34Z

Summary

This PR fixes our e2e tests with a few minor changes to ensure they're reliable again.

Changes

These are the changes worth noting:

fix missing type keyword for import which caused Playwright test runner to fail (see error screenshot)
revert redirect route accidentally removed here
remove unnecessary test
update fork commit to fix integrated terminal tests

Screenshot of Error

Test Gauntlet Results

We missed these errors because we lost faith in the end-to-end testing CI job due to flakiness in the past. In order to ensure our tests pass The Gauntlet, we ran them in all 3 major browsers x10 each. Here is a chart of the results:

	3 browsers (x10 + 1 worker)
globalSetup	✅
codeServer > should navigate to home page	✅
codeServer > should always see the code-server editor	✅
codeServer > should always have a connection	✅
codeServer > should show the Integrated Terminal	✅
login > should see the login page	✅
login > should be able to login	✅
login > should see an error message for missing password	✅
login > should see an error message for incorrect password	✅
login > should hit the rate limiter for too many unsuccessful logins	✅
openHelpAbout > should see code-server version in about dialog	✅
Terminal > should echo a string to a file	✅

Therefore, we can now mark the test-e2e job as required before PRs merge.

TODOs

clean up this PR description
- add gauntlet chart
note the regressions (thank you for catching them tests)
update commit for fork
adjust settings > required CI checks > test-e2e
open issue for more gauntlet stuff Add e2e testing gauntlet + scaffold #4320

Debugging Notes

Although @bryphe-coder and I used git bisect and narrowed it down to this PR/commit: #4010, it appears something downstream/upstream may have changed leading to this break. I couldn't figure out what it was but I was able to narrow the error down to the file changed in this PR. It's weird because our tests passed when that PR was added so I'm not sure what changed? Either way, happy it's fixed now.

How I found it

After git bisect helped me determine the offending file, I then went to the commit before it with git checkout beebf53adc0a7c51d63c27b4981a4b381334b820 and then one by one, I checked out different files (55 files changed)until I figured out which one it was.

Steps:

git bisect until you find bad commit
git checkout <commit-SHA> of good commit before bad commit
git checkout <bad-commit-SHA> -- <file> one by one until finding the bad file.

Note: I also sped up the process by checking out whole directories i.e. git checkout d8c344beda423d4af131ad213e982a4f4dc6387c src. That's how I realized it was in the /src directory.

From there, I commented out different lines until I figured out what was wrong.

Turns out all the login tests are failing because we removed the redirect logic here.

Ah yeah default worker count is cores / 2 and CI has 2 cores
so 1 worker

Should be fine to specify this I think in the playwright config.

Relevant Links:

Fixes #4342

codecov · 2021-10-04T23:25:55Z

Codecov Report

Merging #4310 (02f4e00) into main (271bc06) will decrease coverage by 2.00%.
The diff coverage is 62.91%.

@@            Coverage Diff             @@
##             main    #4310      +/-   ##
==========================================
- Coverage   68.22%   66.21%   -2.01%     
==========================================
  Files          31       31              
  Lines        1586     1619      +33     
  Branches      308      316       +8     
==========================================
- Hits         1082     1072      -10     
- Misses        430      469      +39     
- Partials       74       78       +4

Impacted Files	Coverage Δ
src/common/emitter.ts	`100.00% <ø> (ø)`
src/node/cli.ts	`82.00% <ø> (-0.30%)`	⬇️
src/node/constants.ts	`100.00% <ø> (ø)`
src/node/entry.ts	`0.00% <0.00%> (ø)`
src/node/routes/login.ts	`81.81% <0.00%> (+10.90%)`	⬆️
src/node/util.ts	`79.23% <12.50%> (-0.55%)`	⬇️
src/node/routes/vscode.ts	`50.00% <16.66%> (+12.06%)`	⬆️
src/node/coder_cloud.ts	`27.27% <25.00%> (-1.30%)`	⬇️
src/node/main.ts	`47.57% <60.52%> (+2.22%)`	⬆️
src/node/http.ts	`55.68% <78.26%> (+6.42%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 271bc06...02f4e00. Read the comment docs.

github-actions · 2021-10-04T23:30:21Z

✨ Coder.com for PR #4310 deployed! It will be updated on every commit.

Host: https://codercom-57flsjczd-codercom.vercel.app/docs/code-server
Last deploy status: success
Commit: 02f4e00
Workflow status: https://github.com/cdr/code-server/actions/runs/1396197310

bryphe-coder

Wow, insidious.... Nice work finding this!

jsjoeio · 2021-10-05T22:32:59Z

While the test runner is fixed, the e2e tests are either failing or timing out

See logs: https://github.com/cdr/code-server/pull/4310/checks?check_run_id=3808423259#step:10:37

I took a look at the failed videos. It appears code-server isn't even starting up. Converting back to draft so I can fix this.

This test was originally added to ensure playwright was working. At this point, we know it works so removing this test because it doesn't help with anything specific to code-server and only adds unnecessary code to the codebase plus increases the e2e test job duration.

I don't know if it's a resources issue, playwright, or code-server but it seems like the e2e tests choke when multiple workers are used. This change is okay because our CI runner only has 2 cores so it would only use 1 worker anyway, but by specifying it in our playwright config, we ensure more stability in our e2e tests working correctly. See these PRs: - #3263 - #4310

src/node/constants.ts

src/node/routes/vscode.ts

test/playwright.config.ts

vendor/package.json

jsjoeio · 2021-10-08T19:30:36Z

Hmm, the tests are still failing 🤔

Thinking out loud, the main difference is that when I run them locally, I run yarn watch which builds code-server and then the tests run against that vs. in CI, it runs against release-packages.

I'm currently working with @code-asher to see if I can reproduce locally on macOS.

EDIT: Yup, we can reproduce locally

Our code has new dependencies on VS Code that are pulled in when the unit tests run. Because of this we need to build VS Code before running the unit tests (as it only pulls built code).

This resolves a security report with one of its dependencies (vm2).

This is necessary now that we import from the out directory.

These were renamed so the cached paths need to be updated. I changed the key as well to force a rebuild.

This way it works for local testing as well. I had to use out-build instead of out-vscode-server-min because Jest throws some obscure error about a handlebars haste map.

It contains fixes for missing files in the build.

Shares code with the test HTTP server. For now it is a function but maybe we should make it a class that is extended by tests.

Unfortunately the logger currently chokes when provided with error objects. Also for some reason the bracketed text was not displaying...

The address was recently changed to use URL which seems to add a trailing slash when using toString, causing the regex match to fail.

This is used to set cookies when using a base path.

The file this was testing no longer exists.

Since this is a web path and not platform-dependent.

jsjoeio self-assigned this Oct 4, 2021

jsjoeio added ci Issues related to ci testing Anything related to testing labels Oct 4, 2021

jsjoeio added this to the 3.12.1 - improve release process milestone Oct 4, 2021

jsjoeio marked this pull request as ready for review October 5, 2021 21:33

jsjoeio requested a review from a team as a code owner October 5, 2021 21:33

bryphe-coder approved these changes Oct 5, 2021

View reviewed changes

jsjoeio marked this pull request as draft October 6, 2021 19:36

greyscaled approved these changes Oct 6, 2021

View reviewed changes

jsjoeio added 5 commits October 7, 2021 12:41

fix(testing): revert change & fix playwright tests

d1e36dd

fix(constants): add type to import statement

53743af

revert(vscode): add missing route with redirect

4805186

jsjoeio force-pushed the jsjoeio-fix-playwright branch from 783a1d6 to 3f48ba7 Compare October 7, 2021 19:41

jsjoeio commented Oct 7, 2021

View reviewed changes

src/node/constants.ts Show resolved Hide resolved

src/node/routes/vscode.ts Show resolved Hide resolved

test/playwright.config.ts Outdated Show resolved Hide resolved

vendor/package.json Outdated Show resolved Hide resolved

jsjoeio requested review from greyscaled and bryphe-coder October 7, 2021 19:47

greyscaled approved these changes Oct 7, 2021

View reviewed changes

jawnsy mentioned this pull request Oct 8, 2021

Remove unused Lock config #4304

Merged

jsjoeio force-pushed the jsjoeio-fix-playwright branch from 3f48ba7 to 910a7cd Compare October 8, 2021 17:46

chore(vscode): update to latest fork

0b080f5

jsjoeio force-pushed the jsjoeio-fix-playwright branch from 910a7cd to 0b080f5 Compare October 8, 2021 20:09

GirlBossRush added 2 commits October 15, 2021 17:30

Touch up compilation step.

d7e7e43

Bump vendor.

717bb79

GirlBossRush assigned code-asher Oct 15, 2021

GirlBossRush force-pushed the jsjoeio-fix-playwright branch from d5b4f64 to b4bda04 Compare October 21, 2021 17:06

GirlBossRush and others added 7 commits October 21, 2021 13:17

Update tests to reflect new upstream behavior.

353c2a1

Move unit tests to after the build

c1b40a5

Our code has new dependencies on VS Code that are pulled in when the unit tests run. Because of this we need to build VS Code before running the unit tests (as it only pulls built code).

Upgrade proxy-agent dependencies

fd42387

This resolves a security report with one of its dependencies (vm2).

Symlink VS Code output directory before unit tests

dd8124f

This is necessary now that we import from the out directory.

Fix issues surrounding persistent processes between tests.

6efc7b4

Update VS Code cache directories

6d777c1

These were renamed so the cached paths need to be updated. I changed the key as well to force a rebuild.

Move test symlink to script

64b6f71

This way it works for local testing as well. I had to use out-build instead of out-vscode-server-min because Jest throws some obscure error about a handlebars haste map.

code-asher force-pushed the jsjoeio-fix-playwright branch 3 times, most recently from 8fb7340 to 8a9fe9a Compare October 26, 2021 15:46

code-asher added 5 commits October 26, 2021 16:35

Fix listening on a socket

752fe7c

Update VS Code

0cfd245

It contains fixes for missing files in the build.

Standardize disposals

51a48ba

Dispose HTTP server

fc81fc3

Shares code with the test HTTP server. For now it is a function but maybe we should make it a class that is extended by tests.

Dispose app on exit

070d131

code-asher force-pushed the jsjoeio-fix-playwright branch from 8a9fe9a to 070d131 Compare October 26, 2021 21:22

Fix logging link errors

a343efe

Unfortunately the logger currently chokes when provided with error objects. Also for some reason the bracketed text was not displaying...

code-asher force-pushed the jsjoeio-fix-playwright branch from f297b7a to f8528fa Compare October 27, 2021 21:22

code-asher added 2 commits October 27, 2021 21:37

Update regex used by e2e to extract address

8857e67

The address was recently changed to use URL which seems to add a trailing slash when using toString, causing the regex match to fail.

Log browser console in e2e tests

7a52cb8

code-asher force-pushed the jsjoeio-fix-playwright branch from f8528fa to 7a52cb8 Compare October 27, 2021 21:42

code-asher added 4 commits October 28, 2021 18:43

Add base back to login page

44602cc

This is used to set cookies when using a base path.

Remove login page test

6fbbd83

The file this was testing no longer exists.

Use path.posix for static base

f4a8c9d

Since this is a web path and not platform-dependent.

Add test for invalid password

02f4e00

code-asher merged commit 705e821 into main Oct 28, 2021

code-asher deleted the jsjoeio-fix-playwright branch October 28, 2021 20:27

jsjoeio modified the milestones: 3.12.1 - improve release process, 4.0.0 Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(testing): revert change & fix playwright tests #4310

fix(testing): revert change & fix playwright tests #4310

jsjoeio commented Oct 4, 2021 •

edited by code-asher

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

github-actions bot commented Oct 4, 2021 •

edited

Loading

bryphe-coder left a comment

jsjoeio commented Oct 5, 2021

jsjoeio commented Oct 8, 2021 •

edited

Loading

fix(testing): revert change & fix playwright tests #4310

fix(testing): revert change & fix playwright tests #4310

Conversation

jsjoeio commented Oct 4, 2021 • edited by code-asher Loading

Summary

Changes

Screenshot of Error

Test Gauntlet Results

TODOs

Debugging Notes

How I found it

codecov bot commented Oct 4, 2021 • edited Loading

Codecov Report

github-actions bot commented Oct 4, 2021 • edited Loading

bryphe-coder left a comment

Choose a reason for hiding this comment

jsjoeio commented Oct 5, 2021

jsjoeio commented Oct 8, 2021 • edited Loading

jsjoeio commented Oct 4, 2021 •

edited by code-asher

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

github-actions bot commented Oct 4, 2021 •

edited

Loading

jsjoeio commented Oct 8, 2021 •

edited

Loading